feat: implement light client verification methods #1116
Conversation
🦋 Changeset detectedLatest commit: 6aed942 The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Sounds like a new package makes sense here, what do you think?
Why not add the new types to the existing serialization map? I'll concede it's not a great fit in
Can the tests not live in this repo?
Seems like it could just live in |
I don't have an opinion here, but I can move it into it's own package! Just didn't want to add any package bloat if it was against the goals of the refactoring that's going on :)
Because there is only one overlapping type, it wasn't clear that having the larger map would be a clear win. This type of refactor sounds like something best split from this PR. Do you mind if it's handled separately and I open an issue for this?
They could, but the scripts are meant to be more non-deterministic to sample the chain to give confidence in the impl, and the tests vectors and framework I'm just starting might add bloat to the repo. After I'm done with it, we can revisit and see if we want? It also shouldn't take too long, so could wait for that if it's a blocker. Edit: also the other main reason is that this framework is intended to be used across different language impls, and not tied to this specific one
I don't think there is because the param is just the response type for the light client header and data. I don't think it's even feasible to manually construct that. I could be missing a use case here though. |
|
I made some of the changes. I'll make the following outstanding changes tomorrow (listing mainly for my own memory):
|
|
So the light client verification test was moved to the new Alternatively, I could decouple this and just write a new test that checks a few pre-defined execution proofs, so it depends on what types of tests are preferred here. The current test is conflicting with #1120, so if the intention is to remove all NAJ tests forever, then this might be the only way to go. |
The way I've been handling this is to add |
Initially I was thinking it made sense in
I see, I missed that
Definitely! I was more curious whether you had anything in particular that couldn't be accommodated here. |
Nope, was assuming you wouldn't want to include a submodule in this repo with all of the test vectors (it probably will grow to a decent size). The runner logic is pretty light (https://github.com/austinabell/near-light-client-tests/blob/main/scripts/runBlockTestVectors.ts) and could be moved here pretty easily, and just have that be executed outside of the current |
|
Update on this: I added a runner in rainbow-bridge to validate the logic against the shared test suite here Near-One/rainbow-bridge#897. Hope that gives confidence in the implementation. I can move the runner logic from https://github.com/austinabell/near-light-client-tests/blob/main/scripts/runExecutionTestVectors.ts, https://github.com/austinabell/near-light-client-tests/blob/main/scripts/runBlockTestVectors.ts into a test in this repo, similar to that PR, if preferred for more test coverage on this logic. If so, let me know how you'd like that set up :) |
| } | ||
|
|
||
| const blockProducers: ValidatorStakeView[] = currentBlockProducers; | ||
| if (newBlock.approvals_after_next.length < blockProducers.length) { |
There was a problem hiding this comment.
The python implementation checks for strict equality. The spec uses a zip of the two arrays. Should we also assert the lengths are equal?
There was a problem hiding this comment.
That implementation is wrong. The spec says, and in practice, there are blocks with more block producers than approvals because in the case of validator rotations, both the old and new bps must be included.
I can find and give a block where this is the case if needed, but I don't have an example off-hand.
There was a problem hiding this comment.
Gotcha, I trust your judgment here. Thanks!
There was a problem hiding this comment.
References, for posterity, to remove the trust aspect for others 😄 : https://github.com/near/NEPs/blob/master/specs/ChainSpec/LightClient.md#signature-verification. Also, line in rainbow bridge implementation, which is used in production https://github.com/aurora-is-near/rainbow-bridge/blob/33ca808b45cb5e9cf2e27f741b0f6e42d97c276b/contracts/eth/nearbridge/contracts/NearBridge.sol#L212 (python client just used for tests)
Also, anyone would be able to switch this to strict equal and see that it fails on recent blocks.
packages/light-client/src/block.ts
Outdated
| const borshBps: BorshValidatorStakeView[] = bps.map((bp) => { | ||
| if (bp.validator_stake_struct_version) { | ||
| const version = parseInt( | ||
| bp.validator_stake_struct_version.slice(1) |
There was a problem hiding this comment.
how about the more strict (and safer) check: bp.validator_stake_struct_version === "V1"
packages/light-client/src/borsh.ts
Outdated
| ], | ||
| }, | ||
| ], | ||
| ]); |
There was a problem hiding this comment.
nit: consistency on newline after file
| @@ -0,0 +1,44 @@ | |||
| module.exports = function getConfig(env) { | |||
There was a problem hiding this comment.
Is there a specific reason why we're writing tests in js and not ts?
There was a problem hiding this comment.
no idea, was following the format of existing tests when I wrote
| throw new Error('Enum can only take single value'); | ||
| } | ||
| Object.keys(properties).map((key: string) => { | ||
| (this as any)[key] = properties[key]; |
|
|
||
| export class BorshEmpty extends Assignable {} | ||
|
|
||
| export class BorshPartialExecutionStatus extends Enum { |
There was a problem hiding this comment.
This is not a comment for this PR, but a general one for the borsh-js design. I think the design is rather inflexible as it requires that serializables are classes, and that makes it very unnatural that we have to use Enum and Assignable.
I'd imagine something like this is more reasonable:
export type BorshPartialExecutionStatus = (
{unknown: {}} | {failure: {}} | {successValue: Uint8Array} | {successReceiptId: Uint8Array});
const SCHEMA_BorshEmpty: Schema<{}> = {
kind: 'struct',
fields: [],
};
const SCHEMA_BorshPartialExecutionStatus: Schema<BorshPartialExecutionStatus> = {
kind: 'enum',
field: 'enum',
values: [
['unknown', SCHEMA_BorshEmpty],
['failure', SCHEMA_BorshEmpty],
['successValue', ['u8']],
['successReceiptId', [32]],
],
};
// to deserialize:
const executionStatus = deserialize(SCHEMA_BorshPartialExecutionStatus, buffer);
if ('successValue' in executionStatus) { useBuffer(executionStatus.successValue); }
// to serialize (no need to wrap):
const buffer = serialize(SCHEMA_BorshPartialExecutionStatus, {failure: {}});But that requires changing borsh-js.
There was a problem hiding this comment.
Yeah, I agree. Was a large initiative and unrelated to my changes though, so I won't be addressing here.
|
Hey @austinabell, is this PR ready to be merged? As far as I understand most comments have been addressed and has went through the eyes of several people. If it's good I can resolve the merge conflicts and get it merged. |
Hi @vikinatora, it's out of my control when and how this PR gets merged, but feel free to resolve merge conflicts if you'd like! |
|
@frol, I believe we can proceed with merging in this case. What do you think? |
|
@vikinatora Let's get it merged and iterate from there 🙏 |
|
@austinabell can I have write permission to your branch? I believe it's unnecessary to fork your fork and create a new PR just to resolve the conflicts. We'd make it harder to track the conversations here and context of the PR. |
gave access! Let me know if you need anything else! |
|
CI node is down and tests aren't passing. We just need to wait out for it to be up again and we can merge I believe |
Pre-flight checklist
pnpm changesetto create achangesetJSON document appropriate for this change.Motivation
Part of the implementation for https://near.social/#/devgovgigs.near/widget/gigs-board.pages.Post?id=148
This enables NAJ to verify light client blocks and execution proofs. The API is designed to be low-level and unopinionated. There can definitely be higher-level APIs built on this to automatically sync and verify transactions or just be more ergonomic, but anything I could think of came with tradeoffs. This might give some context as to why the API surfaces (specifically
validateExecutionProof) are the way they are -- for flexibility and extensibility.Open questions/comments:
transactionspackage, so thePublicKeyschema type is duplicated in the changes. There are also a bunch of types added that might seem unnecessary, but it's just because the borsh-js API is very limitinganyuntil fix: add missing fields to execution outcome and test #1113 comes invalidateLightClientBlocktaking last known block lite view and block producers separately as params is intentional. These might be kept separately, and the RPC marks returning bp set as optional (although the current nearcore RPC never returns null)validateExecutionProoftaking themerkleRootas bytes is intentional. One might not be using the JS light client to pull theheader->merkleRootor just have this root encoded in a different way. Bytes is most unopinionated here, instead of having to know that it is a base58 encoded string from the RPC response. This also keeps the API more stable if the data is deserialized properly in the future.computeBlockHashcould be a utility that is moved outside of this module, but might be confusing if it exists in another namespace. Possibly re-export it from utilities/provider?Test Plan
Added tests here, building out a test framework for this now.
Related issues/PRs
#1113 is the only open one relating to this. All others have been pulled in.